feat(pr-reviewer): read the repo's AGENTS.md and guideline docs before reviewing - #474
Conversation
…e reviewing The PR reviewer did not reliably see a repository's own guidance. The cron automation clones the target repo into a workspace subdirectory, so its AGENTS.md is never loaded into the agent's context as an always-on repo skill; the reviewer had no instruction to read it either. Tell both reviewer prompt surfaces — the automation's _build_review_prompt and the /codereview plugin prompt — that they MUST read the repository's AGENTS.md (root + any nested one covering the changed files) plus other relevant docs (CONTRIBUTING.md, CLAUDE.md, .cursorrules, review/coding guidelines) and apply that guidance. This lets a repo steer reviews via AGENTS.md, matching how it steers working agents. Co-authored-by: smolpaws <engel@enyst.org>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review
Taste Rating: 🟡 Acceptable — works, but introduces two overlapping mechanisms for the same goal.
Summary
This PR adds an instruction in both reviewer prompts (plugins/pr-review/scripts/prompt.py:81 and skills/github-pr-reviewer/scripts/main.py:811) telling the spawned reviewer agent to autonomously read AGENTS.md (and other guideline docs) before reviewing. Existing tests are extended with substring assertions.
What works
- The prompt wording is clear and consistent across the two prompts that needed updating.
- The
must readsemantics are intentionally strong, which is the right call given how often AI reviewers otherwise skip repo conventions. - The test additions are appropriate for prompt-template changes (asserting key substrings on the rendered string).
- No new external dependencies; no security or breaking-change risk; pure prompt-string edit.
Concerns (minor — not blocking)
-
Duplicate prose across two files (the inline comments flag both spots). The "MUST read AGENTS.md..." paragraph is now duplicated verbatim between
prompt.pyandmain.py. If the list of files ever changes (SECURITY.md,STYLE.md, etc.), both files have to be updated in lockstep — and the test substring list will silently drift out of sync. Consider extracting one shared constant (e.g.REPO_GUIDANCE_INSTRUCTION) and concatenating it into both prompts. -
Overlaps with the existing
REPO_REVIEW_GUIDE_PATHmechanism inskills/github-pr-reviewer/scripts/main.py(_load_repo_review_guide, lines ~752 and theguide_sectioninjection). That mechanism already deterministically reads.agents/skills/custom-codereview-guide.mdand injects its contents into the prompt. This PR adds a second, non-deterministic overlay that asks the spawned agent to do similar discovery on its own. The two paths have overlapping but not identical scope. A cleaner design would be to extend the host script (the deterministic path) to also walk the changed-file paths and inject any matchingAGENTS.md(root + nested) as a single<repository_guidance>block — eliminating the runtime cost, the latency, and the non-determinism of asking the agent to do the discovery. -
Hardcoded step numbering churn in
main.py— inserting a new "step 2" required renumbering every subsequent step (1→2 ... 8→9). The cross-reference to the fallback (fallback in step 5) was updated tostep 6, so it's currently consistent, but the underlying smell is that the prompt has hardcoded numbers that have to be hand-maintained on every reorder. If a step is ever inserted or removed again, a number will silently disagree.
These are suggestions for a follow-up; they don't block merging this PR.
Testing
test_prompt_requires_reading_repository_guidanceandtest_prompt_instructs_reading_repo_guidanceassert on substring presence in the rendered prompt. Appropriate for prompt-template changes; not behavioral coverage.- The previous behaviour (guide section injection,
_load_repo_review_guidepaths) is unchanged and previously tested.
Risk & Safety
- 🟢 LOW. Prompt-only change in a non-production-critical path. Worst case: a reviewer conversation spends a few extra tool calls reading
AGENTS.md, or follows a stale guideline and produces a slightly different review. No data loss, no security regression, no user-visible product change.
Verdict
✅ Worth merging. The behaviour described in the title works as advertised. The duplication and overlap-with-existing-mechanism concerns are worth a follow-up but don't justify holding this PR.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review
Taste Rating: 🟡 Acceptable — works, but introduces two overlapping mechanisms for the same goal.
Summary
This PR adds an instruction in both reviewer prompts (plugins/pr-review/scripts/prompt.py:81 and skills/github-pr-reviewer/scripts/main.py:811) telling the spawned reviewer agent to autonomously read AGENTS.md (and other guideline docs) before reviewing. Existing tests are extended with substring assertions.
What works
- The prompt wording is clear and consistent across the two prompts that needed updating.
- The
must readsemantics are intentionally strong, which is the right call given how often AI reviewers otherwise skip repo conventions. - The test additions are appropriate for prompt-template changes (asserting key substrings on the rendered string).
- No new external dependencies; no security or breaking-change risk; pure prompt-string edit.
Concerns (minor — not blocking)
-
Duplicate prose across two files (the inline comments flag both spots). The "MUST read AGENTS.md..." paragraph is now duplicated verbatim between
prompt.pyandmain.py. If the list of files ever changes (SECURITY.md,STYLE.md, etc.), both files have to be updated in lockstep — and the test substring list will silently drift out of sync. Consider extracting one shared constant (e.g.REPO_GUIDANCE_INSTRUCTION) and concatenating it into both prompts. -
Overlaps with the existing
REPO_REVIEW_GUIDE_PATHmechanism inskills/github-pr-reviewer/scripts/main.py(_load_repo_review_guide, lines ~752 and theguide_sectioninjection). That mechanism already deterministically reads.agents/skills/custom-codereview-guide.mdand injects its contents into the prompt. This PR adds a second, non-deterministic overlay that asks the spawned agent to do similar discovery on its own. The two paths have overlapping but not identical scope. A cleaner design would be to extend the host script (the deterministic path) to also walk the changed-file paths and inject any matchingAGENTS.md(root + nested) as a single<repository_guidance>block — eliminating the runtime cost, the latency, and the non-determinism of asking the agent to do the discovery. -
Hardcoded step numbering churn in
main.py— inserting a new "step 2" required renumbering every subsequent step (1→2 ... 8→9). The cross-reference to the fallback (fallback in step 5) was updated tostep 6, so it's currently consistent, but the underlying smell is that the prompt has hardcoded numbers that have to be hand-maintained on every reorder. If a step is ever inserted or removed again, a number will silently disagree.
These are suggestions for a follow-up; they don't block merging this PR.
Testing
test_prompt_requires_reading_repository_guidanceandtest_prompt_instructs_reading_repo_guidanceassert on substring presence in the rendered prompt. Appropriate for prompt-template changes; not behavioral coverage.- The previous behaviour (guide section injection,
_load_repo_review_guidepaths) is unchanged and previously tested.
Risk & Safety
- 🟢 LOW. Prompt-only change in a non-production-critical path. Worst case: a reviewer conversation spends a few extra tool calls reading
AGENTS.md, or follows a stale guideline and produces a slightly different review. No data loss, no security regression, no user-visible product change.
Verdict
✅ Worth merging. The behaviour described in the title works as advertised. The duplication and overlap-with-existing-mechanism concerns are worth a follow-up but don't justify holding this PR.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Inline-comment debugging test.
3 participants
HUMAN:
This PR proposes to nudge the reviewer agent to read the contribution guidelines in the repo under review, e.g. AGENTS.md including nested, if any, or other common .md files.
Why
Current
mainnow gives the automation an exact PR checkout as its workspace and can inject one configured review-guide file. That covers the preferred custom guide when it exists, but it does not replace discovery of root and nestedAGENTS.mdfiles or other repository guidance such asCONTRIBUTING.md.The
/codereviewplugin prompt likewise did not explicitly tell the reviewer to inspect repository-owned guidance. Both review surfaces should make that discovery step explicit so project rules consistently influence automated reviews.Summary
AGENTS.md, any nestedAGENTS.mdcovering changed files, and other relevant contribution/review docs before inspecting the PR./codereviewplugin prompt.Issue Number
Fixes #519
How to Test
uv run --group test pytest -q tests/- 780 passed, 14 skipped.uv run pytest -q skills/github-pr-reviewer/tests/test_main.py- 52 passed.uv run python scripts/sync_extensions.py --check- passed; reports the existing non-blockingissue-duplicate-checkermarketplace warning.uv run python scripts/sync_openhands_sdk_skill.py --check- up to date.npm run build:automationsandnpm run build:skills- regenerated checked-in artifacts.git diff --check- clean.Video/Screenshots
N/A - prompt/guidance change with no UI surface.
Notes
After updating from main, CI's live SDK-doc sync also detected the newly published
58_ask_oracle_toolexample. The prescribed generator refreshedskills/openhands-sdk/SKILL.mdandskills/index.js; no SDK documentation was edited by hand.